Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

array::try_map #79713

Closed
wants to merge 4 commits into from
Closed

array::try_map #79713

wants to merge 4 commits into from

Conversation

eopb
Copy link
Contributor

@eopb eopb commented Dec 4, 2020

TI: #79711

pub fn try_map<F, R, E, U>(self, mut f: F) -> Result<[U; N], E> where F: FnMut(T) -> R, R: Try<Ok = U, Error = E> { ... }

Thanks to @lcnr for helping me on this PR.

Much of the code was copied from array::map.

@rust-highfive
Copy link
Collaborator

r? @withoutboats

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 4, 2020
@eopb eopb mentioned this pull request Dec 4, 2020
4 tasks
@withoutboats
Copy link
Contributor

Thanks for the PR. I haven't been involved in the decisions to add any of the relevant methods for evaluating this one, so I want to consult with the rest of the libs teams on this. Specifically, I think we need to develop more policy around these questions:

  1. What guidelines do we use to determine if its worthwhile to add a try_ variant of a higher order function?
  2. What guidelines do we use to determine if its worthwhile to add iterator-like combinators directly to array?
  3. This uses the Try bound, which I notice, so do the stable Iterator try methods; however, it necessarily also returns a Result, rather than R, because its not possible to return R, due to lack of HKT. Should we have the R parameter at all, or just limit this to Result? (fwiw I was somewhat surprised to discover the Iterator methods use a Try bound and I'd like some consensus around that even without this issue)

@rust-lang/libs

@scottmcm
Copy link
Member

A few off-the-cuff musings,

  1. I think it would make sense for there to be at least one "fallible constructor" kind of thing for arrays. Whether that's try_map, I don't know -- it could be Add 'core::array::from_fn' and 'core::array::try_from_fn' #75644 try_generate instead, or something else, or it might be fine to have both. It's straight-forward to do generate as <[(); N]>::map or map as generate-with-mutable-closure, so multiple doesn't seem strictly necessary.
  2. A rule similar to whether to have them on Option seems plausible to me -- which I guess is only an argument for map, not for try_map. Hmm, try_map is similar to .map().transpose() on Option, so perhaps transpose: [Result<T, E>; N] -> Result<[T; N], E> could be argued for, but I guess that doesn't quite work the same as it has different short-circuiting.
  3. I agree it would be undesirable for a closure producing Option<U>s to end up with a Result<[U; N], NoneError> from this method. Looks like the GAT need has come up on the Iterator::try_find tracking issue too, and something like that would be nice, but it's probably far off so only using Result would probably be better near-term.

@bors
Copy link
Contributor

bors commented Dec 22, 2020

☔ The latest upstream changes (presumably #79451) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-9 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
extracting /checkout/obj/build/cache/2020-11-19/rustfmt-nightly-x86_64-unknown-linux-gnu.tar.xz
error: failed to read `/checkout/src/tools/cargo/crates/credential/cargo-credential-1password/Cargo.toml`

Caused by:
  No such file or directory (os error 2)
Build completed unsuccessfully in 0:00:15
make: *** [prepare] Error 1
Makefile:60: recipe for target 'prepare' failed
Command failed. Attempt 2/5:
Command failed. Attempt 2/5:
error: failed to read `/checkout/src/tools/cargo/crates/credential/cargo-credential-1password/Cargo.toml`

Caused by:
  No such file or directory (os error 2)
Build completed unsuccessfully in 0:00:00
make: *** [prepare] Error 1
Makefile:60: recipe for target 'prepare' failed
Command failed. Attempt 3/5:
Command failed. Attempt 3/5:
error: failed to read `/checkout/src/tools/cargo/crates/credential/cargo-credential-1password/Cargo.toml`

Caused by:
  No such file or directory (os error 2)
Build completed unsuccessfully in 0:00:00
make: *** [prepare] Error 1
Makefile:60: recipe for target 'prepare' failed
Command failed. Attempt 4/5:
Command failed. Attempt 4/5:
error: failed to read `/checkout/src/tools/cargo/crates/credential/cargo-credential-1password/Cargo.toml`

Caused by:
  No such file or directory (os error 2)
Build completed unsuccessfully in 0:00:00
make: *** [prepare] Error 1
Makefile:60: recipe for target 'prepare' failed
Command failed. Attempt 5/5:
Command failed. Attempt 5/5:
error: failed to read `/checkout/src/tools/cargo/crates/credential/cargo-credential-1password/Cargo.toml`

Caused by:
  No such file or directory (os error 2)
Build completed unsuccessfully in 0:00:00
make: *** [prepare] Error 1
Makefile:60: recipe for target 'prepare' failed
The command has failed after 5 attempts.

@bors
Copy link
Contributor

bors commented Jan 1, 2021

☔ The latest upstream changes (presumably #80566) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon
Copy link
Member

Ping from triage - @ethanboxx
can you please resolve the merge conflict and build failure?

@rustbot label: -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 19, 2021
@JohnCSimon JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Feb 14, 2021
@JohnCSimon
Copy link
Member

Triage:
@ethanboxx - unfortunately I'm closing this as inactive. Thanks for contributing. If you're willing to continue on this please open a new PR.

@JohnCSimon JohnCSimon closed this Feb 14, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 3, 2021
Make `array::{try_from_fn, try_map}` and `Iterator::try_find` generic over `Try`

Fixes rust-lang#85115

This only updates unstable functions.

`array::try_map` didn't actually exist before; this adds it under the still-open tracking issue rust-lang#79711 from the old PR rust-lang#79713.

Tracking issue for the new trait: rust-lang#91285

This would also solve the return type question in for the proposed `Iterator::try_reduce` in rust-lang#87054
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants